Merged
Conversation
Use app-provided cache directory for os.CreateTemp instead of os.TempDir() which resolves to /data/local/tmp/ on Android — a directory not writable by regular apps. Thread TempDir through GeneratorDependencies -> BundleGenerator and MobileDependency -> EngineConfig so the Android client can pass its cache directory from PlatformFiles.CacheDir().
Move log collection into platform-dispatched addPlatformLog(): - Android: dumps logcat buffer via /system/bin/logcat -d - Other platforms: existing file-based log + systemd fallback
Accept PlatformFiles parameter so debug bundle can be generated even when the engine is not running by loading config from disk. Pass anonymize flag from the UI.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Android debug-bundle creation and upload, persists cacheDir and config on Client with concurrency protection, threads a temp/cache dir from PlatformFiles → ConnectClient → MobileDependency → Engine → debug generator, and splits platform log collection into Android and non-Android implementations. Changes
Sequence DiagramsequenceDiagram
participant Client as Android Client
participant Connect as ConnectClient
participant Engine as Engine
participant Generator as BundleGenerator
participant Upload as Management / Upload
Client->>Connect: RunOnAndroid(..., stateFile, cacheDir)
Connect->>Engine: setup (EngineConfig.TempDir := cacheDir)
Client->>Generator: NewBundleGenerator(deps{TempDir := cacheDir, ...})
Client->>Generator: Generate(anonymize?, include sync/metrics)
Generator->>Generator: addPlatformLog() (logcat or logfile/fallback)
Generator->>Generator: createArchive() -> file in TempDir
Generator-->>Client: bundle file path
Client->>Upload: POST bundle -> cfg.ManagementURL (2m timeout)
Upload-->>Client: upload response / key
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/android/client.go`:
- Around line 200-220: DebugBundle currently ignores
PlatformFiles.StateFilePath(), causing the bundle generator to use the default
service-manager state.json; update the DebugBundle function to pass
platformFiles.StateFilePath() into debug.GeneratorDependencies (add a StateFile
or StateFilePath field) and ensure the code path that calls addStateFile()
consumes that field (modify addStateFile to accept the provided state path
instead of resolving the default). Change references in DebugBundle, the
debug.GeneratorDependencies struct, and addStateFile() to thread
platformFiles.StateFilePath() through to the bundle creation.
- Around line 254-256: The call to debug.UploadDebugBundle currently uses
context.Background() which can block indefinitely; change it to use a
caller-provided context (e.g., pass ctx into the surrounding function) or wrap a
provided context with a timeout using context.WithTimeout and cancel; replace
debug.UploadDebugBundle(context.Background(), ...) with
debug.UploadDebugBundle(ctx, ...) or debug.UploadDebugBundle(ctxWithTimeout,
...) and ensure you defer cancel() and propagate the context to callers so
uploads are bounded.
In `@client/android/platform_files.go`:
- Around line 7-10: You widened the exported PlatformFiles interface by adding
CacheDir(), which is a breaking change for existing Android embedders; revert
PlatformFiles to only expose ConfigurationFilePath() and StateFilePath(), and
introduce a new interface (e.g., PlatformFilesWithCache or PlatformFilesV2) that
adds CacheDir(); update call sites to detect and use CacheDir() via a type
assertion against the new interface (or provide an adapter/helper that falls
back safely) rather than changing the original PlatformFiles API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c91e1ecc-7fe1-4885-be7f-9a4ee0f92a69
📒 Files selected for processing (8)
client/android/client.goclient/android/platform_files.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/debug/debug_android.goclient/internal/debug/debug_nonandroid.goclient/internal/engine.goclient/internal/mobile_dependency.go
Comment on lines
+200
to
+220
| func (c *Client) DebugBundle(platformFiles PlatformFiles, anonymize bool) (string, error) { | ||
| cfg := c.config | ||
| cacheDir := c.cacheDir | ||
|
|
||
| // If the engine hasn't been started, load config from disk | ||
| if cfg == nil { | ||
| var err error | ||
| cfg, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ | ||
| ConfigPath: platformFiles.ConfigurationFilePath(), | ||
| }) | ||
| if err != nil { | ||
| return "", fmt.Errorf("load config: %w", err) | ||
| } | ||
| cacheDir = platformFiles.CacheDir() | ||
| } | ||
|
|
||
| deps := debug.GeneratorDependencies{ | ||
| InternalConfig: cfg, | ||
| StatusRecorder: c.recorder, | ||
| TempDir: cacheDir, | ||
| } |
Contributor
There was a problem hiding this comment.
StateFilePath() is still ignored for Android debug bundles.
DebugBundle receives platformFiles, but only uses the config path and cache dir. The bundle generator still resolves state.json from the default service-manager location, so Troubleshoot bundles created from the Android app will miss the real sandboxed state file. Please thread platformFiles.StateFilePath() into GeneratorDependencies and make addStateFile() use it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/android/client.go` around lines 200 - 220, DebugBundle currently
ignores PlatformFiles.StateFilePath(), causing the bundle generator to use the
default service-manager state.json; update the DebugBundle function to pass
platformFiles.StateFilePath() into debug.GeneratorDependencies (add a StateFile
or StateFilePath field) and ensure the code path that calls addStateFile()
consumes that field (modify addStateFile to accept the provided state path
instead of resolving the default). Change references in DebugBundle, the
debug.GeneratorDependencies struct, and addStateFile() to thread
platformFiles.StateFilePath() through to the bundle creation.
Collaborator
Author
There was a problem hiding this comment.
On Android, addStateFile() calls NewServiceManager("").GetStatePath() which resolves to /var/lib/netbird/state.json (the Linux default), not the app's actual state file path that Java passes via PlatformFiles.StateFilePath(). The bundle silently skips the state file since that path doesn't exist on Android.
However, this is a pre-existing issue in the upstream debug.go, not something introduced by this PR. The fix would involve adding a StatePath field to GeneratorDependencies and threading it through addStateFile() with a fallback to the current ServiceManager lookup when empty. I'll address this in a follow-up to keep this PR focused.
Contributor
There was a problem hiding this comment.
@pappz, thanks for the detailed explanation — that confirms this is a pre-existing gap in debug.go, not a regression introduced here. The fix approach you've described (adding a StatePath field to GeneratorDependencies with a fallback to NewServiceManager("").GetStatePath() when empty) sounds right. Happy to keep this PR focused and defer to a follow-up.
Would you like me to open a GitHub issue to track this? It would capture the context: addStateFile() on Android resolves to /var/lib/netbird/state.json via NewServiceManager instead of the app-sandbox path from PlatformFiles.StateFilePath(), and the fix involves threading StatePath through GeneratorDependencies.
✏️ Learnings added
Learnt from: pappz
Repo: netbirdio/netbird PR: 5888
File: client/android/client.go:200-220
Timestamp: 2026-04-14T17:54:53.373Z
Learning: In netbirdio/netbird, `addStateFile()` in `client/internal/debug/debug.go` calls `NewServiceManager("").GetStatePath()` which resolves to `/var/lib/netbird/state.json` (the Linux default). On Android, this path does not exist; the real state file path is provided by `PlatformFiles.StateFilePath()`. The fix is to add a `StatePath` field to `GeneratorDependencies` and thread it through `addStateFile()` with a fallback to the `ServiceManager` lookup when empty. This is a pre-existing issue in the upstream debug.go (not introduced by PR `#5888`) and is deferred to a follow-up PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5826
File: client/internal/portforward/manager.go:35-38
Timestamp: 2026-04-08T14:55:48.251Z
Learning: In `client/internal/portforward/manager.go` (netbirdio/netbird), persisting permanent-lease (ttl=0/UPnP error 725 fallback) port mapping state for crash-recovery cleanup is intentionally deferred to a follow-up PR. The blocker is that `State.Cleanup` requires NAT gateway re-discovery via `nat.DiscoverGateway`, which can block startup for ~10 seconds when no gateway is present, affecting all clients. The TODO comment at line ~35 documents this constraint. Do not flag the missing state persistence for permanent leases as a blocking issue in this PR.
Learnt from: pappz
Repo: netbirdio/netbird PR: 5888
File: client/android/platform_files.go:7-10
Timestamp: 2026-04-14T17:51:30.929Z
Learning: In the netbirdio/netbird repository, treat the Android client API in `client/android/**/*.go` (e.g., exported interfaces like `PlatformFiles`) as having no versioning or stability guarantees, since it is not a semver-stable public API. Because the repo pins explicit submodule versions, widening these exported interfaces (such as adding new methods to `PlatformFiles`) should not be reviewed/flagged as a breaking API change. Only flag changes where they break behavior within the pinned implementation, not where the interface is merely extended.
Learnt from: siriobalmelli
Repo: netbirdio/netbird PR: 5504
File: client/internal/dns/host_darwin.go:75-79
Timestamp: 2026-03-13T09:37:39.641Z
Learning: In netbirdio/netbird, `stateManager.UpdateState()` failures are handled with log.Errorf/log.Warnf and execution continues (never return an error). The sole error path is `errStateNotRegistered`, which is a programming bug not a runtime condition. All call sites across routemanager, engine_ssh, dns/systemd_linux, dns/network_manager_unix, dns/resolvconf_unix, dns/host_windows, and updatemanager follow this log-and-continue convention. Do not flag log-and-continue on UpdateState as a bug in this codebase.
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 5441
File: management/server/user.go:745-749
Timestamp: 2026-02-24T19:32:13.189Z
Learning: In Go codebases like netbirdio/netbird, methods returning (T, error) should follow the convention: if an error is returned, propagate it and return early. When the caller handles the error immediately, explicit nil checks on the returned value are unnecessary; rely on the error to guard control flow and only inspect the value when err is nil. This guidance applies broadly to Store methods such as GetUserByUserID in Go code; prefer early return on error and avoid redundant nil checks after a successful error path.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5530
File: proxy/internal/udp/relay.go:276-278
Timestamp: 2026-03-07T13:59:16.506Z
Learning: For Go 1.25+ projects, use sync.WaitGroup.Go() to run goroutines because it handles Add(1) and defer Done() automatically. Do not flag wg.Go(...) as a compilation error in codebases that require Go 1.25 or newer. If supporting older Go versions, gate such usage with build tags or avoid wg.Go() to maintain compatibility; verify the module targets Go 1.25+ before adopting this pattern.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/forwarder/icmp.go:268-287
Timestamp: 2026-03-25T09:21:53.814Z
Learning: When using gVisor’s `header.ICMPv6Checksum(ICMPv6ChecksumParams)` (`gvisor.dev/gvisor/pkg/tcpip/header`), the pseudo-header checksum is computed internally from `Src`, `Dst`, and `Header`. Use `PayloadCsum`/`PayloadLen` only for scattered-buffer cases where the ICMPv6 payload bytes are not included in `Header`. If `Header` already contains the complete ICMPv6 message (ICMPv6 header + all data), call `ICMPv6Checksum` with `PayloadCsum: 0` and `PayloadLen: 0` (or omit those fields). Do not pass a separately computed pseudo-header checksum (e.g., `header.PseudoHeaderChecksum(...)`) as `PayloadCsum` in that case, because it will be double-counted and produce an incorrect checksum.
Use a 2-minute context timeout instead of context.Background() to prevent the upload from blocking indefinitely.
lixmal
reviewed
Apr 16, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
8-8: Consider pinning golangci-lint to a specific version for better reproducibility.The repo config is already v2-compatible (
version: "2"in.golangci.yaml), but@latestin the local Makefile makes lint results depend on install date. Note that the CI workflow also usesversion: latest, so local installs will stay in sync with CI, but neither is pinned to a fixed release. If stricter reproducibility is desired, pin both the Makefile and CI to the same version (e.g.,v2.0.0or later).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 8, Replace the unpinned go install reference that uses `@latest` in the Makefile (the GOBIN=$(shell pwd)/bin go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest invocation) with a specific release tag (e.g., `@v1.56.0` or another chosen v2.x.y) to ensure reproducible linting; update the same pinned version in your CI workflow to keep local and CI installs in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Line 8: Replace the unpinned go install reference that uses `@latest` in the
Makefile (the GOBIN=$(shell pwd)/bin go install
github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest invocation) with a
specific release tag (e.g., `@v1.56.0` or another chosen v2.x.y) to ensure
reproducible linting; update the same pinned version in your CI workflow to keep
local and CI installs in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d223944-dde9-4246-a698-b88f96f706c1
📒 Files selected for processing (2)
Makefileclient/internal/debug/debug_android.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/debug/debug_android.go
config, cacheDir and connectClient were written in Run/RunWithoutLogin on a background Thread and read from the UI thread (Networks) and the TUN looper thread (RenewTun) with no synchronization. A Stop->Run cycle could race with a concurrent DebugBundle or Networks call.
|
lixmal
approved these changes
Apr 20, 2026
MichaelUray
added a commit
to MichaelUray/netbird
that referenced
this pull request
Apr 24, 2026
…-addresses Resolved conflicts against upstream netbirdio#5906 (ios mac filter / network_addr.go extraction) and netbirdio#5888 (DebugBundle on Android client): - client/android/client.go: keep both OnUnderlyingNetworkChanged (ours) and DebugBundle (upstream), replaced non-ASCII arrow in comment with plain text. - client/system/info.go: drop duplicated networkAddresses/isDuplicated - they now live in client/system/network_addr.go after upstream extraction. - client/system/network_addr.go: adopt upstream's toNetworkAddress helper but keep ctx-aware signature + skipNoMacFilter so Android continues to use the external iFace discoverer. - client/system/info_ios.go: add exported NetworkAddresses(ctx) shim so the engine call compiles on ios; the iOS body stays context-free (iOS has no external discoverer).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Add Android debug bundle support with Troubleshoot UI
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Chores